module: support eval with ts syntax detection #56285

marco-ippolito · 2024-12-17T10:41:57Z

Previous attempt: #56273

What is the problem?

Before this PR when --experimental-strip-types was enabled, --eval would always parse the input as typescript.
If unflagged, the typescript parser would throw different errors on invalid syntax so unflagging would become a breaking change.

With this PR when running --eval and --experimental-strip-types is enabled, if parsing the code fails we try again with typescript parser.
If it fails again we throw the original error, adding the typescript parser message.

In this way the error is the original error and it's not a breaking change.

Example:


With `--experimental-strip-types`:


marcoippolito@marcos-MBP node % ./node --experimental-strip-types --input-type=module -e "enum Foo{}"    
file:///Users/marcoippolito/Documents/projects/forks/node/[eval1]:1
enum Foo{}
^^^^

  x TypeScript enum is not supported in strip-only mode
   ,----
 1 | enum Foo{}
   : ^^^^^^^^^^
   `----


SyntaxError: Unexpected reserved word
    at compileSourceTextModule (node:internal/modules/esm/utils:338:16)
    at ModuleLoader.eval (node:internal/modules/esm/loader:218:18)
    at node:internal/process/execution:322:29
    at asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:98:11)
    at Object.runEntryPointWithESMLoader (node:internal/modules/run_main:120:19)
    at evalTypeScriptModuleEntryPoint (node:internal/process/execution:318:47)
    at node:internal/main/eval_string:32:3

Node.js v24.0.0-pre




Without `--experimental-strip-types`:


marcoippolito@marcos-MBP node % ./node  --input-type=module -e "enum Foo{}"
file:///Users/marcoippolito/Documents/projects/forks/node/[eval1]:1
enum Foo{}
^^^^

SyntaxError: Unexpected reserved word
    at compileSourceTextModule (node:internal/modules/esm/utils:338:16)
    at ModuleLoader.eval (node:internal/modules/esm/loader:218:18)
    at node:internal/process/execution:73:24
    at asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:98:11)
    at Object.runEntryPointWithESMLoader (node:internal/modules/run_main:120:19)
    at evalModuleEntryPoint (node:internal/process/execution:72:47)
    at evalTypeScriptModuleEntryPoint (node:internal/process/execution:309:12)
    at node:internal/main/eval_string:32:3

Node.js v24.0.0-pre

This PR also add two new --input-type:

module-typescript
commonjs-typescript

So that if the syntax is known we can reduce the overhead of multiple parsing.
If the -typescript input is passed we can throw ERR_INVALID_TYPESCRIPT_SYNTAX safely

the temporary side effect is to remove the workers ability to eval typescript code, which is currently untested (and it never was tested)

nodejs-github-bot · 2024-12-17T10:42:03Z

Review requested:

@nodejs/loaders
@nodejs/typescript

doc/api/cli.md

GeoffreyBooth

Looking good! I like this approach, it feels like a good way to unflag strip-types and provide convenience to the vast majority of users, while providing an option to be explicit for edge cases.

@joyeecheung should review the additions to lib/internal/process/execution.js, I feel like I’ve worked in that file in the past and she’s had notes for me.

doc/api/cli.md

GeoffreyBooth · 2024-12-17T16:30:09Z

doc/api/cli.md

+Node.js will first try to run the code as JavaScript,
+then it will try to run the code as TypeScript.


We should probably say something about syntax detection here, that first it attempts to run as CommonJS JavaScript, and depending on the parse error thrown, will retry as ESM JavaScript or CommonJS TypeScript; and then potentially a third time as ESM TypeScript? I’m not even sure what the flow would be, which probably means that we should spell it out 😄

I'm not an expert on how the js syntax detection is implemented but the flow is:

run normally as javascript (this will perform the detection by trying cjs and esm)

if invalid syntax, strip the types and try again

if invalid again throw the first invalid syntax error enriched with the typescript message

The details of syntax detection are here: https://github.com/nodejs/node/blob/6012a4e9a2733b25a7023baa0a58bef85c9ebc5f/doc/api/esm.md#:~:text=at%20pjsonURL.-,DETECT_MODULE_SYNTAX,-(source)

The important part is to clarify which specific errors will trigger strip-types behavior. I assume there’s no overlap between the “retry as ESM” errors and the “retry with type stripping” errors, so the flow is no more expensive than the current detection flow: try first as CommonJS JavaScript, and retry as stripped types.

It gets even more complicated because doesn’t the strip-types flow itself use syntax detection? Like after it strips the types, then it initially tries to run as CommonJS, then retries as ESM?

Yes it repeats the detection twice but its trasparent from my implementation. I'm not sure it can be skipped

doc/api/cli.md

lib/internal/process/execution.js

codecov · 2024-12-17T18:55:45Z

Codecov Report

Attention: Patch coverage is 98.14815% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.54%. Comparing base (a50f3d5) to head (0c1d4d0).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options.cc	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56285      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190243   190425     +182     
  Branches    36536    36553      +17     
==========================================
+ Hits       168461   168616     +155     
- Misses      14963    14982      +19     
- Partials     6819     6827       +8

Files with missing lines	Coverage Δ
lib/internal/main/eval_string.js	`84.61% <100.00%> (+4.95%)`	⬆️
lib/internal/modules/cjs/loader.js	`98.11% <ø> (-0.01%)`	⬇️
lib/internal/modules/esm/translators.js	`92.88% <ø> (-0.05%)`	⬇️
lib/internal/modules/typescript.js	`100.00% <100.00%> (ø)`
lib/internal/process/execution.js	`99.18% <100.00%> (+0.38%)`	⬆️
src/node_options.cc	`87.89% <40.00%> (-0.10%)`	⬇️

... and 47 files with indirect coverage changes

lib/internal/process/execution.js

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 17, 2024

marco-ippolito force-pushed the test/wrap-in-syntax-error branch 6 times, most recently from 7229405 to f0f0e3c Compare December 17, 2024 14:44

marco-ippolito added module Issues and PRs related to the module subsystem. strip-types Issues or PRs related to strip-types support labels Dec 17, 2024

marco-ippolito force-pushed the test/wrap-in-syntax-error branch from f0f0e3c to 728a1fd Compare December 17, 2024 14:51

marco-ippolito marked this pull request as ready for review December 17, 2024 14:52

marco-ippolito force-pushed the test/wrap-in-syntax-error branch 4 times, most recently from 3485014 to 1dd1cb5 Compare December 17, 2024 15:16

marco-ippolito requested review from joyeecheung and GeoffreyBooth December 17, 2024 15:19

marco-ippolito commented Dec 17, 2024

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

marco-ippolito commented Dec 17, 2024

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

marco-ippolito commented Dec 17, 2024

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

marco-ippolito force-pushed the test/wrap-in-syntax-error branch from 2f87a9c to aeb77e9 Compare December 17, 2024 15:29

GeoffreyBooth reviewed Dec 17, 2024

View reviewed changes

aduh95 reviewed Dec 18, 2024

View reviewed changes